Add support for custom fonts #421

carson-katri · 2021-07-08T13:16:21Z

Adds a .custom member to Font, which will pass the value to font-family in the DOM renderer.

ezraberch · 2021-07-08T14:28:34Z

Certain characters in the font name, such as ", can break the HTML. It's probably best to sanitize the font names. Not doing so could potentially cause security issues as well.
From https://developer.mozilla.org/en-US/docs/Web/CSS/font-family:

Font family names containing whitespace should be quoted.

You should always include at least one generic family name in a font-family list, since there's no guarantee that any given font is available. This lets the browser select an acceptable fallback font when necessary.

carson-katri · 2021-07-08T15:31:37Z

I added a Sanitizer protocol to TokamakStaticHTML, and created a sanitizer for CSS strings. Please let me know if I overlooked anything in the sanitizer implementation.

ezraberch · 2021-07-08T16:51:43Z

Generic family names are keywords and must not be quoted.

Do we want to allow custom selection of the fallback, or multiple fonts in general? There are several ways this could be done (such as allowing the font name string to contain multiple fonts, allowing an array of font names to be used, or allow chaining of .font() to add fonts rather than replacing).

carson-katri · 2021-07-08T22:45:55Z

I liked the idea of chaining font modifiers, because it's compatible with SwiftUI.

Now a .font modifier on any View/Text will add to a stack of fonts, which will then be translated into CSS.

Renderers can still choose to ignore the _fontStack environment value and use font instead

Text("Hello, world!")
  .font(.custom("Marker Felt", size: 17))
  .font(.system(.body, design: .serif))

The above snippet will use Marker Felt if available in the UA, or the default serif stack if not:

font-family: 'Marker Felt', Cambria, 'Hoefler Text', Utopia, ...;

I also improved the sanitizer to sanitize identifiers and strings separately.

MaxDesiatov · 2021-07-09T12:55:09Z

Sources/TokamakCore/Tokens/Font.swift

+        _fontPath = [newFont] + _fontPath.filter { $0 != newFont }
+      } else {
+        _fontPath = []
+      }
    }


Could this file be split by any chance to avoid the linter warning? Maybe some extensions or types moved to separate files?

Sure, I'll split it into more manageable files.

MaxDesiatov

Seems legit 👍

ezraberch · 2021-07-09T13:55:35Z

Sources/TokamakStaticHTML/Sanitizer.swift

+        input.starts(with: "'") || input.starts(with: "\"")
+          && !input.dropFirst().dropLast().allSatisfy(isStringContent)
+          && input.last == input.first


What is the purpose of this function? It isn't called anywhere. Additionally, the logic isn't correct.

Suggested change

input.starts(with: "'") || input.starts(with: "\"")

&& !input.dropFirst().dropLast().allSatisfy(isStringContent)

&& input.last == input.first

(input.starts(with: "'") || input.starts(with: "\""))

&& input.dropFirst().dropLast().allSatisfy(isStringContent)

&& input.last == input.first

ezraberch · 2021-07-09T13:55:39Z

Sources/TokamakStaticHTML/Sanitizer.swift

+    /// Sanitizes a quoted string.
+    enum StringValue: Sanitizer {
+      static func isStringContent(_ char: Character) -> Bool {
+        char != "'" && char != "\""


Newline and backslash can also cause problems.

Suggested change

char != "'" && char != "\""

!"'\"\n\\".contains(char)

ezraberch · 2021-07-09T13:55:41Z

Sources/TokamakStaticHTML/Sanitizer.swift

+      static func sanitize(_ input: String) -> String {
+        input.filter(isIdentifierChar)
+      }


There are additional restrictions for the first character of identifiers. I'm not sure if it really matters here, but I'd at least add a comment in case this code gets reused.

I updated the sanitizers to use the CSS 2.1 grammar.

ezraberch · 2021-07-09T13:55:58Z

Sources/TokamakDemo/TextDemo.swift

@@ -64,6 +64,13 @@ struct TextDemo: View {
        )
        .multilineTextAlignment(alignment)
      }
+      Text("Custom Font")
+        .font(.custom("\"Marker Felt\"", size: 17))


The font-family generated by this has just the custom font. There should always be a fallback, even if one isn't specified explicitly.

I added a default fallback of .body.

MaxDesiatov

Thanks

ezraberch · 2021-07-09T16:45:30Z

Identifier validation is still not properly handling when the first character is a number. Try adding this test:
XCTAssertFalse(Sanitizers.CSS.validate(identifier: "1hey-there"))
String sanitize truncates rather than removing the offending characters. For example
Sanitizers.CSS.sanitize(string: "'hey'there'") -> "\'hey\'"
Is identifier-sanitize still necessary? It only happens if identifier-validate has already succeeded, so I don't think it does anything.

carson-katri · 2021-07-09T18:54:24Z

Resolved 3. Do you have suggestions on how to fix the other 2? I can't figure out why its matching numbers in the identifier parsing.

ezraberch · 2021-07-09T20:44:53Z

The reason why it's matching some numbers is because it's matching this part:
/// '[\240-\377]'
static let nonAscii: RegularExpression = #"[\240-\377]"#
The digits which match are 0,1,2,3,4, and 7, so it looks like it's ignoring the backslashes and interpreting that as "2, 4, 0-3, 7, or 7".
In other words, swift is not recognizing that syntax as a way to denote a range of characters.
What your filter is doing is finding all matches to the regex and merging them together, and ignoring the rest. You've defined a match as including start and end quotes. So 'ab'ed' has one valid match ('ab') so that's what you get. On the other hand, 'ab'c'de' has two valid matches ('ab' and 'de'), so you get 'ab''de'. Anything not part of a match is lost.
If you remove the requirement that each match includes the start and end quotes, what you're doing should work.

carson-katri · 2021-07-10T18:02:21Z

Thank you for tracking those issues down! Turns out NSRegularExpression uses \0xxx to specify an Octal character, so that fixed it.

MaxDesiatov · 2021-07-12T11:14:01Z

@ezraberch would you have a moment to have another quick look? I personally don't have any objections for this code to be merged, but I appreciate very detailed reviews from you!

ezraberch

Functionality looks good now.

I'm still unsure why Identifier.sanitize and StringValue.validate exist, as they're not called anywhere (other than by tests). The Sanitizer protocol does require them, but that seems unnecessary as well.

carson-katri · 2021-07-12T12:49:36Z

Yeah it might be unnecessary. I just thought the functionality may be useful in the future when we are sanitizing more areas.

MaxDesiatov

Thanks!

MaxDesiatov · 2021-07-12T16:18:25Z

@ezraberch thanks again for your feedback on this and other PRs and your previous contributions! Would you be interested in joining our maintainers team? This would allow you to give review approvals for example.

ezraberch · 2021-07-13T18:02:45Z

Sure.

Add support for custom fonts

78b43f4

carson-katri added the SwiftUI compatibility Tokamak API differences with SwiftUI label Jul 8, 2021

carson-katri added 2 commits July 8, 2021 11:22

Add CSS sanitizer

0443ec0

Add sanitizer tests

9644bc0

Add fallback font families

60a80d9

carson-katri marked this pull request as draft July 8, 2021 17:02

carson-katri added the DOM/HTML renderer Tokamak in the browser label Jul 8, 2021

Add fallback font support by chaining modifiers

c2dff01

carson-katri marked this pull request as ready for review July 8, 2021 22:42

Fix fallback ordering and update tests to not depend on Text

4932798

carson-katri requested a review from a team July 9, 2021 12:51

Cleanup

a7f7810

MaxDesiatov reviewed Jul 9, 2021

View reviewed changes

Split up Font.swift

d0439bb

MaxDesiatov previously approved these changes Jul 9, 2021

View reviewed changes

ezraberch requested changes Jul 9, 2021

View reviewed changes

ezraberch mentioned this pull request Jul 9, 2021

Sanitation of User Input #423

Open

Update sanitizers to use grammar from w3.org, provide default fallback

2a08948

carson-katri dismissed MaxDesiatov’s stale review via 2a08948 July 9, 2021 15:37

MaxDesiatov previously approved these changes Jul 9, 2021

View reviewed changes

Remove sanitize call after validation

9e0986f

carson-katri dismissed MaxDesiatov’s stale review via 9e0986f July 9, 2021 18:53

Fix sanitizier behavior

d91dacf

carson-katri requested a review from a team July 10, 2021 18:10

ezraberch approved these changes Jul 12, 2021

View reviewed changes

MaxDesiatov approved these changes Jul 12, 2021

View reviewed changes

carson-katri merged commit b6790c5 into main Jul 12, 2021

carson-katri deleted the custom-font branch July 12, 2021 16:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for custom fonts #421

Add support for custom fonts #421

carson-katri commented Jul 8, 2021

ezraberch commented Jul 8, 2021

carson-katri commented Jul 8, 2021

ezraberch commented Jul 8, 2021

carson-katri commented Jul 8, 2021 •

edited

Loading

MaxDesiatov Jul 9, 2021

carson-katri Jul 9, 2021

MaxDesiatov left a comment

ezraberch Jul 9, 2021

ezraberch Jul 9, 2021

ezraberch Jul 9, 2021

carson-katri Jul 9, 2021

ezraberch Jul 9, 2021

carson-katri Jul 9, 2021

MaxDesiatov left a comment

ezraberch commented Jul 9, 2021

carson-katri commented Jul 9, 2021 •

edited

Loading

ezraberch commented Jul 9, 2021

carson-katri commented Jul 10, 2021

MaxDesiatov commented Jul 12, 2021

ezraberch left a comment

carson-katri commented Jul 12, 2021

MaxDesiatov left a comment

MaxDesiatov commented Jul 12, 2021

ezraberch commented Jul 13, 2021

Add support for custom fonts #421

Add support for custom fonts #421

Conversation

carson-katri commented Jul 8, 2021

ezraberch commented Jul 8, 2021

carson-katri commented Jul 8, 2021

ezraberch commented Jul 8, 2021

carson-katri commented Jul 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxDesiatov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxDesiatov left a comment

Choose a reason for hiding this comment

ezraberch commented Jul 9, 2021

carson-katri commented Jul 9, 2021 • edited Loading

ezraberch commented Jul 9, 2021

carson-katri commented Jul 10, 2021

MaxDesiatov commented Jul 12, 2021

ezraberch left a comment

Choose a reason for hiding this comment

carson-katri commented Jul 12, 2021

MaxDesiatov left a comment

Choose a reason for hiding this comment

MaxDesiatov commented Jul 12, 2021

ezraberch commented Jul 13, 2021

carson-katri commented Jul 8, 2021 •

edited

Loading

carson-katri commented Jul 9, 2021 •

edited

Loading